Decode old-style nested Xcom value #31866

utkarsharma2 · 2023-06-13T05:45:29Z

Xcom values generated by airflow 2.5.2 was not getting decoded by airflow 2.6.
There were two issues

I think it was a typo, which was mentioned in the issue -

airflow/airflow/serialization/serde.py

Line 273 in 58fca5e

return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
When we have __type == dict in _convert() we don't need to wrap the data in dict again.

closes: #31769

airflow/serialization/serde.py

tests/utils/test_json.py

phanikumv

Test docker-compose quick start is failing, other than that it lgtm

jedcunningham · 2023-06-15T17:59:15Z

airflow/serialization/serde.py

@@ -276,7 +276,11 @@ def deserialize(o: T | None, full=True, type_hint: Any = None) -> object:
 def _convert(old: dict) -> dict:
    """Converts an old style serialization to new style."""
    if OLD_TYPE in old and OLD_DATA in old:
-        return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}
+        # Added to handle for handling xcom data from airflow 2.5.2


Suggested change

# Added to handle for handling xcom data from airflow 2.5.2

# Added to handle for handling xcom data from airflow 2.5.2

This comment feels a little weird to me. Is this really just 2.5.2, or <2.6?

same thought (comment)

I'm not sure what all airflow version generates this encoded value, but I'll check and update that here. Sorry, should have done this in 1st iteration.

I think the code comment should be applicable for 2.4>= to <2.6 because the dataset concept was introduced in 2.4. Will update accordingly. Will update the code comment accordingly.

Also, I was able to replicate the issue for airflow 2.4.3 as running example dag dataset_produces_1 produces data - [{“__type": "airflow.datasets.Dataset", "__source": "dataset_produces_1.producing_task_1", "__var": {"__var": {"uri": "s3://dag1/output_1.txt", "extra": {"__var": {"hi": "bye"}, "__type": "dict"}}, "__type": "dict"}}] and the issue was not caught by testcase - https://github.com/apache/airflow/blob/main/tests/serialization/test_serde.py#L259-L273 as it doesn't have more than two levels of nested __var

hussein-awala

I can't find any encoder which add __var in a nested object __var, and why it was moved from

    @staticmethod
    def _convert(old: dict) -> dict:
        """Converts an old style serialization to new style"""
        if OLD_TYPE in old and OLD_SOURCE in old:
            return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}

to

def _convert(old: dict) -> dict:
    """Converts an old style serialization to new style"""
    if OLD_TYPE in old and OLD_DATA in old:
        return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA][OLD_DATA]}

    return old

in #31866.
I wonder if it was a bug in that PR and we need just to remove it instead of adding a condition for 2.5.2.
BTW, this PR was released in 2.6.0, so we will have the same problem with all the previous version, maybe we need to test 2.5.3 and 2.4.3 before merging this one, in order to update the comments:

# Added to handle for handling xcom data from airflow 2.5.2

@bolkedebruin wdyt?

utkarsharma2 · 2023-06-21T15:04:29Z

@hussein-awala In Airflow 2.4.3 if you run dag dataset_produces_1 it will produce xcom data as below, with nested __var -

{
        "__type": "airflow.datasets.Dataset",
        "__source": None,
        "__var": {
            "__var": {
                "uri": uri,
                "extra": {
                    "__var": {"hi": "bye"},
                    "__type": "dict"
                }
            },
            "__type": "dict",
        },
}

And I think old[OLD_DATA][OLD_DATA] was done to directly get to the second nested __var assuming there will be no more nested __var in serialized data but that is not the case as extra can have dicts too.

Also, in the old test case, we had only tests for the below data -
test case - https://github.com/apache/airflow/blob/main/tests/serialization/test_serde.py#L259-L273

{
    "__type": "airflow.datasets.Dataset",
    "__source": None,
    "__var": {
        "__var": {
            "uri": uri,
            "extra": None              # <-- change 
        },
        "__type": "dict",
    },
}

old[OLD_DATA][OLD_DATA] fails with extra = {"__var": {"hi": "bye"}, "__type": "dict"} and will works with extra=None

But if we explicitly handle the case for dict and do not assume the nested levels of __var below code will work in all cases.

def _convert(old: dict) -> dict:
    """Converts an old style serialization to the new style.""
    if OLD_TYPE in old and OLD_DATA in old:
        #Added to handle for handling xcom data from airflow < 2.6
        if old[OLD_TYPE] == "dict":
            return old[OLD_DATA]
        else:
            return {CLASSNAME: old[OLD_TYPE], VERSION: DEFAULT_VERSION, DATA: old[OLD_DATA]}
    return old

tests/serialization/test_serde.py

bolkedebruin · 2023-06-29T09:38:24Z

Looking

bolkedebruin · 2023-06-29T10:16:29Z

_convert was created to handle old (pre 2.6) XCom seralized data, but the issue is that the new deserializer cannot handle:

{'extra': {'__type': 'dict', '__var': {'hi': 'bye'}}, 'uri': 's3://does_not_exist'}

Which is the nested form of the old style serialization.

I think the solution is correct, but that the commit message and the inline comments could be improved. As mentioned this code was already handling pre 2.6 data, but it wasn't handling the nested version of that correctly. Also I think the test could use some improvement / addition. The above dict should be able to be deseralized directly. So I would expect an additional test.

The deserializer was not properly dealing with nested and wrapped old-style xcom values. --------- Co-authored-by: bolkedebruin <[email protected]> (cherry picked from commit bd32467)

Decode Old Xcom value from airflow 2.5.2

9bac8e5

utkarsharma2 requested review from kaxil, ashb and bolkedebruin as code owners June 13, 2023 05:45

boring-cyborg bot added the area:serialization label Jun 13, 2023

utkarsharma2 marked this pull request as draft June 13, 2023 05:45

utkarsharma2 marked this pull request as ready for review June 13, 2023 06:28

ephraimbuddy added this to the Airlfow 2.6.3 milestone Jun 13, 2023

ephraimbuddy added the type:bug-fix Changelog: Bug Fixes label Jun 13, 2023

phanikumv approved these changes Jun 13, 2023

View reviewed changes

sunank200 approved these changes Jun 13, 2023

View reviewed changes

airflow/serialization/serde.py Outdated Show resolved Hide resolved

potiuk approved these changes Jun 13, 2023

View reviewed changes

ephraimbuddy reviewed Jun 13, 2023

View reviewed changes

airflow/serialization/serde.py Show resolved Hide resolved

vincbeck approved these changes Jun 13, 2023

View reviewed changes

hussein-awala reviewed Jun 14, 2023

View reviewed changes

tests/utils/test_json.py Outdated Show resolved Hide resolved

utkarsharma2 added 3 commits June 15, 2023 11:28

Moved testcase to serialization/test_serde.py

d3eb05e

Added code comment

0dafddd

Remove unwanted changes

1570d9a

utkarsharma2 requested review from hussein-awala and ephraimbuddy June 15, 2023 06:09

phanikumv reviewed Jun 15, 2023

View reviewed changes

utkarsharma2 added 2 commits June 15, 2023 18:14

Merge branch 'main' into DeserializationOldXcom

5e87e3a

Merge branch 'main' into DeserializationOldXcom

45593b9

jedcunningham reviewed Jun 15, 2023

View reviewed changes

hussein-awala reviewed Jun 15, 2023

View reviewed changes

utkarsharma2 added 4 commits June 21, 2023 20:47

Update code comment and add missing test

3a4bd2f

Merge branch 'main' into DeserializationOldXcom

b0de44d

Merge branch 'main' into DeserializationOldXcom

ca1c3f2

Merge branch 'main' into DeserializationOldXcom

6122bcf

utkarsharma2 requested review from hussein-awala, phanikumv and jedcunningham June 23, 2023 13:51

hussein-awala reviewed Jun 25, 2023

View reviewed changes

tests/serialization/test_serde.py Outdated Show resolved Hide resolved

utkarsharma2 requested a review from hussein-awala June 28, 2023 11:31

bolkedebruin added 3 commits June 29, 2023 12:18

Change comment to reflect correct reason

6d282fa

Change to use constant

d3dc6b7

Add tests covering wrapped old style xcom values

d55ecf9

bolkedebruin changed the title ~~Decode old Xcom value from airflow 2.5.2~~ Decode old-style nested Xcom value Jun 29, 2023

bolkedebruin added 3 commits June 29, 2023 12:36

Reinstate backwards compat test

8cc14d2

Fix test

30f282d

Update test_serde.py

11afbe9

bolkedebruin merged commit bd32467 into apache:main Jun 29, 2023

ephraimbuddy mentioned this pull request Jul 7, 2023

Status of testing of Apache Airflow 2.6.3rc1 #32432

Closed

53 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Decode old-style nested Xcom value #31866

Decode old-style nested Xcom value #31866

utkarsharma2 commented Jun 13, 2023 •

edited

Loading

phanikumv left a comment

jedcunningham Jun 15, 2023

hussein-awala Jun 15, 2023

utkarsharma2 Jun 21, 2023 •

edited

Loading

utkarsharma2 Jun 21, 2023

utkarsharma2 Jun 21, 2023

hussein-awala left a comment

utkarsharma2 commented Jun 21, 2023

bolkedebruin commented Jun 29, 2023

bolkedebruin commented Jun 29, 2023

	# Added to handle for handling xcom data from airflow 2.5.2
	# Added to handle for handling xcom data from airflow 2.5.2

Decode old-style nested Xcom value #31866

Decode old-style nested Xcom value #31866

Conversation

utkarsharma2 commented Jun 13, 2023 • edited Loading

phanikumv left a comment

Choose a reason for hiding this comment

jedcunningham Jun 15, 2023

Choose a reason for hiding this comment

hussein-awala Jun 15, 2023

Choose a reason for hiding this comment

utkarsharma2 Jun 21, 2023 • edited Loading

Choose a reason for hiding this comment

utkarsharma2 Jun 21, 2023

Choose a reason for hiding this comment

utkarsharma2 Jun 21, 2023

Choose a reason for hiding this comment

hussein-awala left a comment

Choose a reason for hiding this comment

utkarsharma2 commented Jun 21, 2023

bolkedebruin commented Jun 29, 2023

bolkedebruin commented Jun 29, 2023

utkarsharma2 commented Jun 13, 2023 •

edited

Loading

utkarsharma2 Jun 21, 2023 •

edited

Loading